Skip to content

Update transition time only on new status#238

Merged
Jakob-Naucke merged 1 commit into
trusted-execution-clusters:mainfrom
Jakob-Naucke:transition-time
May 7, 2026
Merged

Update transition time only on new status#238
Jakob-Naucke merged 1 commit into
trusted-execution-clusters:mainfrom
Jakob-Naucke:transition-time

Conversation

@Jakob-Naucke
Copy link
Copy Markdown
Contributor

@Jakob-Naucke Jakob-Naucke commented Apr 10, 2026

Do not update on each new generation

Fixes: #221

@Jakob-Naucke Jakob-Naucke requested a review from alicefr April 10, 2026 10:13
@Jakob-Naucke
Update transition time only on new status
e46b3ad 
Do not update on each new generation

Fixes: trusted-execution-clusters#221

Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
alicefr
alicefr reviewed Apr 14, 2026
View reviewed changes
Comment thread lib/src/lib.rs
) -> Time {
let get = |s: &S| s.conditions().clone();
let conditions = existing_status.as_ref().and_then(get);
let find = |c: &Condition| type_ == c.type_ && new_status == c.status;
Copy link
Copy Markdown
Contributor

@alicefr alicefr Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we need to consider the reason as well?

Copy link
Copy Markdown
Contributor

@alicefr alicefr Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But shouldn't be this looking for condition that changed status? Shouldn't this be new_status != c.status;. What about if the type condition doesn't exist

Copy link
Copy Markdown
Contributor Author

@Jakob-Naucke Jakob-Naucke Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linked issue suggested that it should only change when the condition changes

Copy link
Copy Markdown
Contributor Author

@Jakob-Naucke Jakob-Naucke Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it finds one, it keeps the time of the one it found. If it finds none, it takes now as timestamp.

@openshift-ci openshift-ci Bot added the lgtm label Apr 17, 2026
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 17, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alicefr, Jakob-Naucke

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Jakob-Naucke
Copy link
Copy Markdown
Contributor Author

Jakob-Naucke commented May 7, 2026

Had a successful CI run on #235
/retest

@Jakob-Naucke
Copy link
Copy Markdown
Contributor Author

Jakob-Naucke commented May 7, 2026 •
edited
Loading 

 INFO: test_attestation: Creating VM: test-coreos-vm
[2026-05-07T10:46:25Z ERROR kube_client::client::builder] failed with error client error (Connect)
Error: ServiceError: client error (Connect)
Caused by:
    0: client error (Connect)
    1: tcp connect error
    2: Connection refused (os error 111)
FAILED
test test_parallel_vm_attestation ... Error: Failed to infer configuration: failed to infer config: in-cluster: (failed to read an incluster environment variable: environment variable not found), kubeconfig: (failed to determine current context)

uh, what? cluster just dies? checking for reproducibility

/retest

e: was this a conflict from #248? that's supposed to lock, which worked on the previous host

@alicefr
Copy link
Copy Markdown
Contributor

alicefr commented May 7, 2026 

 INFO: test_attestation: Creating VM: test-coreos-vm
[2026-05-07T10:46:25Z ERROR kube_client::client::builder] failed with error client error (Connect)
Error: ServiceError: client error (Connect)
Caused by:
    0: client error (Connect)
    1: tcp connect error
    2: Connection refused (os error 111)
FAILED
test test_parallel_vm_attestation ... Error: Failed to infer configuration: failed to infer config: in-cluster: (failed to read an incluster environment variable: environment variable not found), kubeconfig: (failed to determine current context)

uh, what? cluster just dies? checking for reproducibility

/retest

e: was this a conflict from #248? that's supposed to lock, which worked on the previous host

Maybe, it run out of memory?

@Jakob-Naucke
Copy link
Copy Markdown
Contributor Author

Jakob-Naucke commented May 7, 2026

@alicefr no signs of OOM but also can't see anything obvious in journal (hard to correlate to an exact moment though because the logs don't log time at that point)… merging for now

@Jakob-Naucke Jakob-Naucke merged commit 356815d into trusted-execution-clusters:main May 7, 2026
9 checks passed
@Jakob-Naucke Jakob-Naucke deleted the transition-time branch May 7, 2026 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alicefr alicefr alicefr approved these changes

Assignees

@alicefr alicefr

Labels

lgtm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] The functions in condition.rs return always a different condition

2 participants

@Jakob-Naucke @alicefr